fix(security): remove allow-same-origin from MCP Apps sandbox iframes#1064
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request restricts the sandbox permissions for MCP app iframes by removing several flags, including allow-same-origin, to improve security. Feedback suggests that while removing allow-same-origin is appropriate, other functional flags such as allow-forms, allow-popups, and allow-modals should be retained to ensure basic application features like form submissions and popups continue to work.
samples/client/lit/custom-components-example/ui/custom-components/mcp-apps-component.ts
Outdated
Show resolved
Hide resolved
The Lit sample and shared sandbox proxy included allow-same-origin in iframe sandbox attributes, violating the MCP Apps guide/spec: - Lit outer iframe: removed sandbox attribute entirely (guide says don't sandbox the proxy iframe) - Lit sendSandboxResourceReady: allow-scripts only (was allow-scripts allow-forms allow-popups allow-modals allow-same-origin) - Shared sandbox.ts inner iframe default: allow-scripts only (was allow-scripts allow-same-origin allow-forms) The Angular sample already correctly used sandbox: 'allow-scripts'. Confirmed against MCP Apps spec (SEP-1865) and AppBridge SDK docs. Fixes security discrepancy found by Gemini Code Assist review on google#1062.
zeroasterisk
force-pushed
the
fix/mcp-apps-sandbox-security
branch
from
508231a to
7fedb1b
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
If the demos continue working fine, let's do it!
(Have you tried this with a deployed version of the app? localhost is often very lax for all these sandboxing options)
| const inner = document.createElement("iframe"); | ||
| inner.style.cssText = "width:100%; height:100%; border:none;"; | ||
| inner.setAttribute("sandbox", "allow-scripts allow-same-origin allow-forms"); | ||
| inner.setAttribute("sandbox", "allow-scripts allow-forms allow-popups allow-modals"); |
There was a problem hiding this comment.
This line is adding "allow-popups" and "allow-modals" without it being documented in the change. Is this intended?
(This is making the allowlist the same as the custom-components-example above so I'm guessing "yes".)
There was a problem hiding this comment.
It was intentional, but I cannot guarantee those intentions :)
2 participants
Summary
This PR updates the iframe sandbox settings for MCP Apps to align with the MCP Apps specification and the A2UI security guide, addressing concerns about over-restriction while ensuring proper isolation. Found during review of PR #1062.
Details of Changes
1. Outer Iframe (Sandbox Proxy)
sandbox="allow-scripts allow-same-origin"to the outer iframe inmcp-apps-component.ts.allow-scriptsandallow-same-originpermissions. The previous draft removed the sandbox attribute entirely based on the A2UI guide's advice to not sandbox the proxy, but this update restores it to strictly comply with the spec.2. Inner Iframe (View)
allow-same-originfrom the inner iframe's sandbox settings in bothmcp-apps-component.tsandsandbox.ts.allow-forms,allow-popups, andallow-modalsflags.allow-same-originensures proper isolation of untrusted content (preventing it from accessing the parent origin). Retaining the other functional flags ensures that apps can still handle forms and popups, avoiding breaking basic functionality as flagged by reviewers. This fits the spec's requirement for "restricted permissions" and follows the A2UI guide's allowed list.Verification Results
allow-same-originbut retains necessary UI capabilities.